Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pricing): expand gateway max price to set per capability/model id #3116

Conversation

ad-astra-video
Copy link
Contributor

@ad-astra-video ad-astra-video commented Aug 3, 2024

This PR allows setting prices per capability and model id.

Current Implementation

Gateways currently use one MaxPrice set at launch with maxPricePerUnit flag and can be adjusted with the setBroadcastConfig endpoint of the cli webserver while the gateway is running. 

Enhancement Requested

Gateways on the AI subnet need the ability to filter orchestrators on price for different pipelines/model ids.  Transcoding would also benefit from this when different encoding codecs are enabled on the network.  The single MaxPrice needs to be updated to enable setting prices per capability (e.g. text-to-image and image-to-video) separately as well as allow the ability to price models within the capabilities separately.  The single MaxPrice will function as the overall default if no prices are set for specific pipelines and/or models.  The separate pricing needs to also allow setting a price per capability for all models in a capability.

Proposed implementation Changes

A maxPricePerCapability launch flag is added to accept json configuration of maximum prices for a capability/model.  The json configuration uses the maxPricePerCapability flag configuration to provide consistency in how prices are set on the network.  The server.BroadcastCfg is updated on the Gateway to store prices per capability/model id that can then be queried to provide the appropriate MaxPrice to use when selecting orchestrators in the selection algorithm.  The server.BroadcastCfg is also extended to add new functions for GetCapabilitiesMaxPrice and GetCapabilityMaxPrice to provide ability to get correct price with provided capabilities of the request. The GetCapabilitiesMaxPrice returns a total MaxPrice for the capabilities included in the request.  If no specific max prices is set for the capabilities requested, the MaxPrice is used.  GetCapabilityMaxPrice returns the max price set for that capability/model id or zero if not set.

The base MaxPrice and SetMaxPrice functions are updated to set pricing of H264 encoding capability to allow extending this to provide different prices for different codecs in the future. ModelID is not used for transcoding so expected to always be "default".

@github-actions github-actions bot added the AI Issues and PR related to the AI-video branch. label Aug 3, 2024
@ad-astra-video ad-astra-video marked this pull request as ready for review August 7, 2024 11:25
@rickstaa rickstaa deleted the branch livepeer:ai-video August 7, 2024 20:53
@rickstaa rickstaa closed this Aug 7, 2024
@rickstaa rickstaa reopened this Aug 7, 2024
@rickstaa rickstaa deleted the branch livepeer:ai-video August 10, 2024 06:53
@rickstaa rickstaa closed this Aug 10, 2024
@rickstaa rickstaa reopened this Aug 10, 2024
@rickstaa rickstaa changed the base branch from ai-video-rebase to ai-video August 10, 2024 15:25
Copy link
Contributor

@eliteprox eliteprox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @ad-astra-video! I tested -maxPricePerCapability with AI model pricing and it works correctly.

I also regression tested -maxPricePerUnit to ensure price controls still work with transcoding. Using the H264 capability as the default price index is a creative approach that should allow us to set prices per codec one day in the future.

I tested livepeer_cli with both options 16, 15 and found the price changes also work correctly there. I made a few edits to setMaxPriceForCapability to prevent a runtime error and updated the test TestMaxPricePerCapability, see my suggestions below.

Overall, changes LGTM!

cmd/livepeer/starter/starter_test.go Outdated Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
server/handlers.go Show resolved Hide resolved
ad-astra-video and others added 2 commits August 15, 2024 23:32
@ad-astra-video
Copy link
Contributor Author

@eliteprox @rickstaa I agree with all suggestions and committed the suggested changes.

Copy link
Contributor

@eliteprox eliteprox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. Approved pending review from @rickstaa

Comment on lines 1831 to 1833
if p.Gateway == "" {
p.Gateway = "default"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this field used for if, from what I understood, this is a config on the gateway itself? I thought this would make sense on an orchestrator config to set prices for specific gateways instead.

Copy link
Contributor Author

@ad-astra-video ad-astra-video Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed Gateway field for now. There was a time that this logic was going to be re-used for the Orchestrator as well to set price per gateway/pipeline/model_id but I think we found a way to re-use the aiModels.json config parsing to set price per gateway at the Orchestrator. Can always add it back in if needed.

Confirmed test still passed.

prices := make([]ModelPrice, len(pricesSet.CapabilitiesPrices))
for i, p := range pricesSet.CapabilitiesPrices {
if p.Gateway == "" {
p.Gateway = "default"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could be nice to support this defaulting for the model_id field as well. Then one can specify only the pipeline for it to be the default price for the pipeline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea. I removed the p.Gateway check and changed it to setting p.ModelID to 'default' if not set specifically.

core/ai.go Outdated
Comment on lines 63 to 64
//no capability description matches name
return Capability_Unused
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be an explicit error instead (or the caller check this Unused value) to avoid silent bad configurations. Otherwise one might be missing a configuration that they swear they are making on the ir JSON config file which could have bad implications (e.g. paying more than they were willing to). In general I think it's better to fail explicitly for bad configs than to ignore them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be better to update this to return (Capability, error) to make it more obvious it should be checked by the caller.

Updated and added a test for this function in ai_test.go

@ad-astra-video
Copy link
Contributor Author

ad-astra-video commented Aug 17, 2024

@victorges @rickstaa updates made for review comments. Small merge conflict on a log line that looked the exact same resolved as well.

ad-astra-video added a commit to ad-astra-video/go-livepeer that referenced this pull request Aug 18, 2024
@rickstaa
Copy link
Contributor

@ad-astra-video the code looks good 🚀. However I was unable to build this on my system 🤔. Could you check if you pushed the latest changes?

GO111MODULE=on CGO_ENABLED=1 CC="" CGO_CFLAGS="" CGO_LDFLAGS=" " go build -o "./" -tags "mainnet" -ldflags="-X github.com/livepeer/go-livepeer/core.LivepeerVersion=0.7.6-cdd74620" cmd/livepeer/*.go
# github.com/livepeer/go-livepeer/server
server/ai_session.go:206:3: undefined: Models
server/ai_session.go:208:29: capabilityConstraints[cap].Models undefined (type *"github.com/livepeer/go-livepeer/core".PerCapabilityConstraints has no field or method Models)
server/ai_session.go:209:14: undefined: core.NewCapabilitiesWithConstraints
server/broadcast.go:102:50: netCaps.CapabilityConstraints undefined (type *"github.com/livepeer/go-livepeer/net".Capabilities has no field or method CapabilityConstraints)
make: *** [Makefile:100: livepeer] Error 1

@ad-astra-video
Copy link
Contributor Author

@rickstaa updated and re-tested this.

docker image build available at docker pull adastravideo/go-livepeer:ai-video-set-max-price-per-capability-rebase

ad-astra-video added a commit to ad-astra-video/go-livepeer that referenced this pull request Aug 21, 2024
@@ -135,6 +135,7 @@ func parseLivepeerConfig() starter.LivepeerConfig {
cfg.OrchPerfStatsURL = flag.String("orchPerfStatsUrl", *cfg.OrchPerfStatsURL, "URL of Orchestrator Performance Stream Tester")
cfg.Region = flag.String("region", *cfg.Region, "Region in which a broadcaster is deployed; used to select the region while using the orchestrator's performance stats")
cfg.MaxPricePerUnit = flag.String("maxPricePerUnit", *cfg.MaxPricePerUnit, "The maximum transcoding price per 'pixelsPerUnit' a broadcaster is willing to accept. If not set explicitly, broadcaster is willing to accept ANY price. Can be specified in wei or a custom currency in the format <price><currency> (e.g. 0.50USD). When using a custom currency, a corresponding price feed must be configured with -priceFeedAddr")
cfg.MaxPricePerCapability = flag.String("maxPricePerCapability", *cfg.MaxPricePerCapability, `json list of prices per capability/model or path to json config file. Use "model_id": "default" to price all models in a pipeline the same. Example: {"capabilities_prices": [{"pipeline": "text-to-image", "model_id": "stabilityai/sd-turbo", "priceperunit": 1000, "pixelsperunit": 1}, {"pipeline": "upscale", "model_id": "default", priceperunit": 1200, "pixelsperunit": 1}]}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the format! Do we want to add the currency field in there as well as an example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to snake case to keep the JSON convention. For this we can also change the PricePerGateway in this format (see https://linear.app/livepeer-ai/issue/AI-574/change-priceperbroadcaster-json-input-keys-to-recommended-conventions).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Will update pricePerGateway to same in separate PR.

mMinGasPrice *stats.Float64Measure
mMaxGasPrice *stats.Float64Measure
mTranscodingPrice *stats.Float64Measure
mTranscodingPricePerCapability *stats.Float64Measure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name it PricePerCapability?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

return price
}

func (cfg *BroadcastConfig) GetCapabilityMaxPrice(cap core.Capability, modelID string) *big.Rat {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one can be lowercase since it is not used outside this file :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

This commit removes the BroadcastConfig initialisation method out of the
starter to keep all logic inside the server module.

return &BroadcastConfig{
maxPricePerCapability: map[core.Capability]map[string]*core.AutoConvertedPrice{
core.Capability_H264: models,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in our chat this can be core.Capability_Unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@@ -1810,6 +1837,58 @@ func getGatewayPrices(gatewayPrices string) []GatewayPrice {
return prices
}

type ModelPrice struct {
Gateway string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this field is a left over of another feature 🤔? Couldn't see it used.

rickstaa and others added 11 commits August 23, 2024 19:22
This commit improves the parsing of the maxPricePerCapability
config so that it doesn't throw nil errors and defaults to the regular
transcoding values. It also throws clear log statments when config values are
incorrect.
This commit simplifies the PipelineToCapability because we don't use
unicode in our capability naming.
This commit applies some minor code improvements.
This commit fixes small typo in the 'price_per_capability' metric.
This commit improves the defaults used in the
setBroadcastMaxPricePerCapability CLI option.
This commit ensures that we unsubscribe to the priceFeed when a model
maxPrice is changed.
This commit applies some common go code conventions to the
NewAISessionSelector function.
This commit optimizes the code a bit.
….com:ad-astra-video/go-livepeer into ai-video-set-max-price-per-capability-rebase
Copy link
Contributor

@rickstaa rickstaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

@rickstaa rickstaa merged commit 4f3947d into livepeer:ai-video Aug 23, 2024
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Issues and PR related to the AI-video branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants